feat(cli): unify external session imports through Host - #5308
Conversation
Current PR design reportThe complete, current design for this PR is versioned in the repository: English · 中文. It covers the user contract, authority and interface seams, source-specific read bounds, pagination and wire limits, import publication and recovery, provider-history admission, error semantics, Desktop/TUI behavior, and verification boundaries. This supersedes the earlier D1–D12 text in this comment. The documentation is aligned with PR HEAD |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head a9142b7bdf9369dc7ba8f63d45b176d39a89b73d.
This change replaces the CLI-local foreign-session handoff with a Runtime Host-owned catalog/import path shared by Claude Code, Codex, and OpenCode, including bounded cursor paging, durable imported Session publication, recovery, and TUI selection. The latest commit also bounds every catalog-row field before wire-budget assembly.
I found two blocking P2 correctness issues:
- outcome-unknown TUI reconciliation can claim a concurrent client import and switch to the wrong Session;
- the Codex filesystem fallback pages by creation-path traversal rather than the previous global update-time order, so recently used or newly archived Sessions can be buried behind stale rows.
Validation completed: clean install, build:test, full typecheck/lint/format/ASF checks, Storage 1266 pass / 11 skip, CLI 1053 pass / 3 skip, focused external-session tests 51/51, and a clean merge tree with current main c22768c3b0dc47518f6f8584e864f86f0b1e5379. Runtime Host full tests were 1932 pass / 19 skip / 1 fail; the only failure was the unchanged managed-Bash sandbox integration because this runner rejects both unshare and bwrap. GitHub currently exposes no hosted checks for this head.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
@hqhq1025 Addressed both findings in 85c7070.
Validated: Storage CodexSessionAdapter 17/17; CLI pi-tui-runner 212/212; targeted TypeScript builds; Biome and I also updated the design note above to reflect the fail-closed unknown-outcome behavior and the globally mtime-ordered Codex fallback. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 85c70702958ca7dcf7f73d62a6b0a6b4739a71c9.
This follow-up makes outcome-unknown TUI imports fail closed and changes the Codex filesystem fallback to globally order active and archived rollouts by file mtime before filtering and paging. Both prior P2 findings are fixed: the TUI no longer attributes a concurrent copy to an uncertain request, and a recently updated old-path or archived rollout is no longer statically buried behind newer creation paths.
One P2 remains in the new fallback ordering: every page recomputes the mutable mtime order while the Host cursor is only a numeric offset. An active rollout that changes between page requests can move ahead of the offset, causing the next page to duplicate a previously shown row and omit the updated Session for the rest of that picker traversal. The inline comment includes a production-path reproduction.
Validation completed on Node 24.18.1: fresh workspace dependency and CLI builds; Storage 1266 pass / 11 skip; CLI 1052 pass / 3 skip; focused Codex adapter 17/17 and TUI paging/outcome-unknown 4/4; changed-file Biome; git diff --check; and a clean merge tree with current main 72cd8b1f532872ef1bcca875a7a7cae4e7b1448e. GitHub reports no hosted checks for this head. I did not exercise native packaged Desktop, Windows/macOS, or a real concurrently-writing Codex process.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for pushing this through, and for the D1–D14 note; being explicit about every deviation from #5053 made the review tractable. Reviewed at head 85c7070. The direction is what we agreed in #5053: the scanner and the digest handoff are gone with nothing dangling, external-session.ts is the old helpers moved rather than rebuilt, the pagination contract holds end to end (I walked the four page-boundary cases; nothing skips or duplicates), D13 lives in the storage authority, restart recovery is wired, and the Codex fallback fix with its regression test checks out. Net production diff is −163 lines. So this is close, and what follows is mostly about how the last rounds of fixes were made rather than about the design.
1. The pattern behind most of what I found. I mapped each issue back to the commit that introduced it, and six of the nine commits are titled "close … findings". Almost every remaining problem comes from a review comment being fixed at the spot the reviewer pointed to instead of at the code that owns the behaviour, and the design note then documents that local choice. Three chains show it:
- A reviewer said "don't publish an empty history" → the importer got a pre-check → the pre-check disagreed with the Ledger → to make them agree, the Ledger's turn rule was forked on
externalOrigin(runtime-ledger-repair.ts:95). That fork is the one real defect here, see point 2. - A reviewer said "bound the reads" → a constant was added in each function under review: the Claude summary completes a cut record with the import's 64 MiB bound, twice, so one 126 MiB transcript takes
listSessionsfrom 57 MiB to 567 MiB resident (measured;claude-code-session-adapter.ts:640-685, and the doc comment at:72-92still says 512 KiB); OpenCode bounds source bytes butconvertTranscriptholds every parsed row, so within the same 64 MiB three JSON shapes measured +155 / +333 / +536 MiB (D3's "~216 MiB" is one corpus, not a bound); and the newdirectorybound drops NULL rows from the picker whilereadSessionstill imports them (opencode-session-adapter.ts:264, wheretitleon the next line already hascoalesce). - A reviewer said "don't infer ownership from a catalog delta" → the TUI went fail-closed, and Desktop's
import-tasks-settings-page.tsx:676-685still does exactly that inference and offers a button into what may be another client's Session. D2 doesn't mention that the two clients now disagree on the same Host outcome.
The ask is not to fix these three one by one. It is to redo the last rounds from the owner outward: for each bound, name the resource it protects (listing memory vs import memory) and put one bound there; for each rule, keep one rule in the authority that owns it and let callers adapt; for each client behaviour, make both clients read the same Host outcome the same way. Then ablate: for every mechanism the fix rounds added, revert it, run the suite, and keep only what fails. From my own ablations, these already don't survive: toCatalogSessionRow's tolerant skip (unreachable, 27/27 pass without it; the SQL already excludes non-text parent_id), the steering clause in isConversationTextMessage (steering rows are filtered before it runs), compareCatalogEntries and the preceding byte in readSummaryTail (dead), and the Codex/Claude parsing helpers' home in @maka/core (each now has exactly one consumer, its adapter; the Adapter is the format authority, so they belong there).
2. The one blocker: an imported Session that opens on an assistant reply can't be continued on Anthropic-family connections. Claude Code and OpenCode transcripts can start with an assistant record; on main the Ledger skipped that turn, so the history the model saw always began at a user boundary. With the D5 fork it is kept, and I traced the real path (SessionManager.sendMessage → new run → buildPriorRuntimeContext → AiSdkBackend with a recording model): the provider receives [assistant, user, assistant, user]. Nothing on that path checks the head; runtime-resume.ts's provider_resume_head_unsupported only runs for explicit resume/fork. @ai-sdk/anthropic groups by role without validating the first one, so messages[0].role is assistant on the wire, which the Messages API rejects, and every retry rebuilds the same head. That is #5053's continue-after-import criterion failing on the most common connection. main's behaviour isn't right either (it silently drops that reply), but the fix belongs where the model history is projected, not in the turn rule: keep the turn in the transcript and have the projection that already owns provider admission handle a non-user head. One rule in the Ledger, one owner for the head. I'd like a real send on an Anthropic connection recorded in the PR once that's in.
3. Tests. About 1,180 test lines were added. Several pin the local fixes above rather than an obligation, and some pass on main unchanged: keeps a transcript that opens on an assistant reply (stubs createImportedSession, asserts only the id), a native transcript turn with no user row is still not converted (a shape no native path produces), two of the three outcome-unknown TUI cases (same assertion as the first), the D7 case (the SQL, not the wrapper, makes it pass), and the (mtime, size) cache case (only size is exercised). Please run the same ablation on the tests: revert the production change each one claims to guard, and drop any that stays green. The ones that do fail on main and go through the owner are good and should stay: the cursor-advances-by-source-rows case, the two importer rejection cases (which, as an aside, also fix a live Desktop defect: main's importer had no emptiness check at all, and the PR undersells that), and the Codex page-order regression.
4. Smaller things to fold into the same pass. The TUI collapses every import failure code except source_limit_exceeded into "Could not import"; model_unavailable and source_unreadable are normal outcomes the protocol defines codes for and Desktop already classifies, so a switch on the code with three strings is enough. The picker scope should come from driver.getWorkspaceTarget() rather than the mutable sessionListScope, otherwise a host-workspace profile can label "current workspace" and query all. externalImportLimitLabel takes string with a default: return kind, which is the token leak D12 exists to prevent. discardCurrentSidePair sits inside the try after a successful switchSession, so a cleanup failure reports "could not open" on a Session that is open. The Codex ORDER BY coalesce(updated_at_ms, updated_at, …) mixes seconds and milliseconds. The benchmark script hand-copies the preflight SQL (they already differ) and its per-session memory column reads a monotone maxRSS, so every row after the first is ~0; if it stays, import the SQL from the adapter. The CHANGELOG entry went under the empty ## Unreleased instead of ## 0.2.0 - Unreleased where every other pending entry is.
Facts: the branch is 10 commits behind main but merges clean, lockfile unchanged, no hosted checks on this head. Storage, core, runtime-ledger, Host coordinator and the TUI external-session suites all pass here.
AI assistance: I used Claude Code to trace the provider path, run the memory measurements and the ablations; conclusions were checked by me.
中文版
感谢把这个推到现在,也感谢 D1–D14 这份说明;把每一处和 #5053 的偏离都写明,评审才有抓手。评审基于 head 85c7070。方向就是 #5053 里定的:scanner 和 digest handoff 删干净了没有残留,external-session.ts 是旧 helper 搬家不是重建,分页契约端到端成立(我走了四种翻页边界情况,不跳行不重复),D13 落在 storage 权威处,重启恢复接好了,Codex fallback 的修复和回归测试也没问题。生产代码净减 163 行。所以离合并不远,下面主要是关于最后几轮修法怎么做的,不是关于设计。
1. 大部分问题背后的同一个模式。 我把每个问题映射回引入它的 commit,九个 commit 里六个标题是「close … findings」。剩下的问题几乎都来自:评审意见在评审者指到的那个位置就地修了,而不是在拥有该行为的代码处修,然后设计说明把这个局部选择记录下来。三条链能看清楚:
- 评审说「不能发空历史」→ importer 加了前置判定 → 判定和 Ledger 不一致 → 为了让两边一致,Ledger 的 turn 规则按
externalOrigin分叉(runtime-ledger-repair.ts:95)。这个分叉就是这次唯一的真缺陷,见第 2 点。 - 评审说「读取要有界」→ 每个被看的函数各加一个常量:Claude 摘要补齐被切断的记录用的是导入的 64 MiB 上限,还补两条,一个 126 MiB 的 transcript 让
listSessions常驻从 57 MiB 涨到 567 MiB(实测;claude-code-session-adapter.ts:640-685,:72-92的注释还写着 512 KiB);OpenCode 界定的是源字节,但convertTranscript同时持有所有解析后的行,同在 64 MiB 内三种 JSON 形状实测 +155 / +333 / +536 MiB(D3 的「~216 MiB」是一份语料的测量,不是上界);新加的directory界让 NULL 行从选择器消失,而readSession照样导入(opencode-session-adapter.ts:264,下一行的title已经用了coalesce)。 - 评审说「不能靠 catalog 差值推断归属」→ TUI 改成 fail-closed,而 Desktop 的
import-tasks-settings-page.tsx:676-685仍在做同样的推断,还给一个按钮打开可能属于另一个客户端的 Session。D2 没提两端现在对同一个 Host 结果的处理不一样了。
我要的不是把这三处逐个修掉,而是从权威往外重做最后几轮:每个界先说清它保护的资源是什么(列表内存还是导入内存),在那里设一个界;每条规则只在拥有它的权威处保留一条,调用方去适配;每个客户端行为,让两端对同一个 Host 结果做同样的解读。然后做消融:修复轮次加的每个机制都还原一次、跑套件,只留会失败的。我自己消融过的这些已经活不下来:toCatalogSessionRow 的容错跳过(不可达,去掉后 27/27 通过;SQL 已排除非文本 parent_id)、isConversationTextMessage 的 steering 子句(steering 行在它之前就被过滤了)、compareCatalogEntries 和 readSummaryTail 里的 preceding 字节(死代码)、Codex/Claude 解析 helper 留在 @maka/core(现在各只有一个消费者,就是对应的 adapter;Adapter 是格式权威,它们该回去)。
2. 唯一的阻塞项:以 assistant 回复开场的导入会话在 Anthropic 系连接上无法继续。 Claude Code 和 OpenCode 的 transcript 可以以 assistant 记录开头;main 上 Ledger 会跳过这个 turn,所以模型看到的历史总是从 user 边界开始。D5 分叉之后它被保留了,我追了真实路径(SessionManager.sendMessage → 新 run → buildPriorRuntimeContext → 带录制 model 的 AiSdkBackend):provider 收到的是 [assistant, user, assistant, user]。这条路径上没有任何 head 校验;runtime-resume.ts 的 provider_resume_head_unsupported 只在显式 resume/fork 时才跑。@ai-sdk/anthropic 按 role 归组不校验首角色,线上请求体 messages[0].role 就是 assistant,Messages API 会拒绝,而每次重试都重建同样的头。这就是 #5053「导入后继续使用」这条验收在最常见连接上失败。main 的行为也不对(它静默丢掉那条回复),但修法应该在投影模型历史的地方,不在 turn 规则里:transcript 里保留这个 turn,由已经拥有 provider 准入的投影层处理非 user 的头。Ledger 一条规则,头只有一个 owner。这一步做完后,希望 PR 里记录一次 Anthropic 连接上的真实发送。
3. 测试。 新增约 1,180 行测试。其中不少钉住的是上面那些局部修法而不是义务,有些在 main 上原样通过:keeps a transcript that opens on an assistant reply(stub 掉了 createImportedSession,只断言 id)、a native transcript turn with no user row is still not converted(本地路径不会产生的形状)、三条 outcome-unknown TUI 用例中的两条(和第一条断言相同)、D7 那条(让它通过的是 SQL 不是包装)、(mtime, size) 缓存那条(只练到了 size)。请对测试做同样的消融:还原每条测试声称守住的生产改动,仍然绿的就删。在 main 上确实失败且经过 owner 的那些是好的,要留:cursor 按源行推进那条、两条 importer 拒绝用例(顺带一提,它们也修了 Desktop 的一个活缺陷:main 的 importer 根本没有空判定,PR 把这点说小了)、Codex 页序回归。
4. 可以并入同一轮的小事。 TUI 把除 source_limit_exceeded 外所有导入失败码折叠成「Could not import」;model_unavailable 和 source_unreadable 是协议定义了码、Desktop 已经分类的正常结果,按码 switch 加三条文案就够。选择器 scope 应来自 driver.getWorkspaceTarget() 而不是可变的 sessionListScope,否则 host-workspace profile 下会标「当前工作区」实际查全部。externalImportLimitLabel 接 string 且 default: return kind,正是 D12 要防的令牌泄漏。discardCurrentSidePair 在 switchSession 成功后还在 try 里,清理失败会对已打开的 Session 报「打不开」。Codex 的 ORDER BY coalesce(updated_at_ms, updated_at, …) 混了秒和毫秒。benchmark 脚本手抄了预检 SQL(已经不一致),逐会话内存列读的是单调的 maxRSS,第一行之后全是 ~0;要留的话从 adapter 导入 SQL。CHANGELOG 条目写进了空的 ## Unreleased,而其他待发布条目都在 ## 0.2.0 - Unreleased 下。
事实:分支落后 main 10 个 commit 但合并干净,lockfile 未变,此 head 没有托管检查。storage、core、runtime-ledger、Host coordinator 和 TUI 外部会话套件在我这里都通过。
AI 辅助:我用 Claude Code 追踪 provider 路径、做内存测量和消融;结论由我核对。
85c7070 to
0a73b37
Compare
|
Implemented in commit 0a73b37 after rebasing onto the current main branch. Review addressed This specifically resolves the remaining P2 from the hqhq1025 review of head 85c7070, in inline discussion #5308 (comment). That review showed that rebuilding the mutable mtime order for every numeric-offset page could duplicate one Session and omit another. This commit does not claim to resolve the later Astro-Han review submitted at 2026-09-15 07:00 UTC, including its assistant-first Anthropic history blocker and broader cleanup requests. Final paging design
Regression coverage The production-path regression creates 20 fallback rollouts, loads page one, changes an unseen rollout to the newest mtime, and then loads page two. The complete traversal keeps the original 20-row order with no duplicate or omission. Tests also cover cursor protocol round-trip, explicit expiry classification, final-page release, and automatic TUI reload. Validation completed: Core, Storage, Runtime Host, and CLI builds; 50 focused protocol, coordinator, and adapter tests; 3 focused TUI tests; TUI copy checks; Biome on all changed files; and git diff check. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 0a73b3794030125ea50a266ed840e251167d94a0.
Relative to the previously reviewed head, the nine existing PR commits are patch-equivalent after rebase and this head adds source-owned catalog cursors, a five-minute in-memory snapshot for the Codex filesystem fallback, explicit cursor-expiration handling, and TUI reload behavior. The prior filesystem mtime/offset finding is fixed for that fallback path.
Two blocking correctness issues remain:
- P1: an imported transcript that begins with an assistant reply is materialized into provider history with that assistant message first. An exact-head production projection plus
@ai-sdk/anthropicrequest probe emitted wire roles["assistant", "user"]; Anthropic-compatible Messages endpoints reject a conversation without a leading user turn, so this imported Session cannot be continued on those connections. - P2: the preferred Codex state-database path still pages a mutable
ORDER BY updated_at...result with a plaino:<offset>cursor. An exact-headHostExternalSessionCoordinator -> CodexSessionAdapter -> node:sqliteprobe returned rows 19 through 04 on page one; after unseen row 01 received a newerupdated_at_ms, page two repeated row 04 and never returned row 01.
Validation on Node 24.18.1: clean npm ci; build:test; Storage 1364 pass / 11 skip; CLI 1053 pass / 3 skip; focused external-session adapter/Host/protocol/TUI tests 271/271; Runtime Ledger repair 13/13; changed-file Biome; ASF headers; git diff --check; and a clean merge tree with current main 3f297e9aaac36b023e219ad3837a790064f592ea. Runtime Host full tests were 1944 pass / 19 skip / 1 fail; the only failure was the managed Bash sandbox integration because this runner rejects both unshare and bwrap, not a failure in the changed path. GitHub exposes no hosted checks for this head. I did not exercise a live Anthropic account or native Windows/macOS clients.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
@Astro-Han The first blocker is corrected in af6c754, and D5 in the design report has been updated to match the code.
Verified: runtime build; SessionManager 190/190; AiSdkBackend 242/242; RuntimeLedgerRepair 12/12; model-history timeline 7/7; Biome and diff checks. A real Anthropic API send has not been run because this environment has no Anthropic credential configured, so I am leaving that acceptance item explicitly open rather than treating the mock provider as equivalent. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 2813eff474d4b9fd4dc4b8ca43cfcb9d36b3f17b.
This follow-up moves assistant-first admission into model-history projection and gives the Codex state-database path the same query-bound snapshot cursor as the filesystem fallback. The previous imported-session Anthropic failure and mutable-offset duplicate/omission are fixed on their intended paths.
Two blocking regressions remain:
- P1: transcript repair now materializes assistant-only turns for native legacy Sessions too, while the new user-boundary projection is restricted to Sessions with
externalOrigin. A native pre-ledger Session therefore produces assistant-first Anthropic wire history and cannot continue. - P2: state-database paging now materializes and path-validates the entire matching catalog before returning the first page, replacing the previous bounded SQL page with unbounded first-page work and retaining up to 32 full snapshots.
Validation on Node 24.18.1: clean npm ci; build:test; Storage 1364 pass / 11 skip; Runtime 3479 pass / 13 skip; CLI 1053 pass / 3 skip; focused changed-path tests 249/249; changed-file Biome; ASF header audit; git diff --check; and a clean merge tree with current main 3f297e9aaac36b023e219ad3837a790064f592ea. GitHub exposes no hosted checks. I did not exercise a live Anthropic account or native Windows/macOS clients; the catalog timing data is from a synthetic local Codex state database.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
资源边界这一组已按 owner 重新整改,见
回归覆盖:catalog 不越过 Claude head window、OpenCode converted-output 超限、NULL directory list/import 对称。Claude 39/39、OpenCode 29/29、Codex 18/18、Core 805/805、Host coordinator 22/22 均通过;完整 Storage 1368 pass / 8 skip / 1 个已知环境失败(Node SQLite ExperimentalWarning 写入 child stderr)。Biome、 D1/D3/D4/D6/D7 设计报告和 PR Verification 已同步为当前实现。 |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 7f4d52f876fb9b943e77353075834e2cb9aefce0.
This follow-up closes both findings from the previous head. Repaired assistant-only history is now admitted at the first model-visible user boundary for native and imported Sessions, while ordinary RuntimeEvent history is preserved. The Codex state-database catalog now keeps a WAL read transaction as the stable snapshot and reads bounded SQL batches instead of materializing the full catalog before returning page one.
I found no remaining P0-P3 issue in the new increment. The 2,000-row first-page regression exercises the bounded path, and the focused production-path coverage includes native repaired history replay and stable state-database pagination.
Validation on Node 24.18.1: clean npm ci; build:test; 228 focused Runtime/Storage tests; git diff --check; and a clean merge tree with current main 3f297e9aaac36b023e219ad3837a790064f592ea. GitHub exposes no hosted checks. I did not exercise a live Anthropic account, sustained concurrent Codex writes while a five-minute read snapshot remains open, or native Windows/macOS clients.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
@Astro-Han Desktop unknown-outcome semantics are now aligned with the TUI in The renderer no longer rereads the external catalog to infer that an import landed or did not land. The public component regression models the exact concurrent-client shape: import returns unknown while a second catalog result contains one new task. It now proves there is only the initial catalog read, the warning remains visible, and no recovered-task button is offered; reverting the production change fails at 2 reads vs 1. Focused results: |
|
Addressed the TUI items from the latest review in
Verification:
|
|
Addressed the Codex mixed-unit catalog ordering item in The state-DB query now normalizes legacy Verification:
I also rechecked and resolved the seven older inline review threads whose production paths and regressions are already fixed. The remaining items from the latest summary review are still tracked separately. |
|
Closed the remaining small review cleanup in
I also rechecked the benchmark concern: the external-session benchmark script is already absent from the branch diff, so the incorrect copied preflight SQL and per-row Verification: |
|
Continued the requested test ablation in
Verification: ExternalSessionImporter + OpenCode + Claude adapter suites: 73/73; storage build, Biome, and |
|
Moved the remaining single-consumer format helpers back to their adapter owners in
I also corrected D8 in the design report to the current rule: external catalog scope comes from the Host workspace target and is independent of the Maka Session picker tab. Verification: Core and Storage typecheck; Core query + Claude + Codex suites 76/76; post-build Claude/Codex suites 59/59; Biome and |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head ae1f5a1ccfea5350eb55e9da0caa1c94ada57dea.
This follow-up removes the unsafe Desktop catalog-delta attribution for outcome-unknown imports, preserves the corresponding TUI outcome and error distinctions, normalizes mixed Codex timestamp units, and moves format-specific parsing helpers back into their adapters. Those changes address the previously raised issues, but the Desktop fail-closed rule can still be bypassed through batch import; I found one P2 inline.
Validation on Node 24.18.1: clean install; build:test; 338 focused Desktop/CLI/Storage tests; full workspace typecheck; changed-file Biome; git diff --check; and a clean merge tree against current main (0d9ea7576a49c2f3173aa2cbe99ded53f82deaab). GitHub exposes no hosted checks. I did not exercise a real commit-outcome disconnect, a live external client, or native packaged Desktop.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for turning this around so quickly. Reviewed at head ae1f5a1, rebased and mergeable. The blocker from last round is fixed at the right owner: one turn rule in RuntimeLedgerRepair, and provider admission in model-history.ts scoped by refs.storedMessageId, which only the repair backfill ever writes. I re-ran the real path (SessionManager.sendMessage → buildPriorRuntimeContext → AiSdkBackend with a recording model) for an imported transcript opening on an assistant reply, a native pre-ledger Session with an assistant-only turn, a thinking-only opening, and the continuation lane: all user-led on the wire. I also tried the coarse rule ("drop everything before the first user event"): 22 genuine ai-sdk-backend failures, because the prior context is routinely a budgeted/folded slice that legitimately starts on an assistant event. So the provenance scope is required, not extra. The session-manager regression goes red with the option off. Every item from points 1, 3 and 4 last round is done as asked. Good.
Two things stand out in this head, and they're the same shape as last time: each review round added machinery and tests at the spot the reviewer pointed to, and the ablation only covered what was named. Production is net −374 against main, which is the right direction, but the Codex adapter went from +100 to +423 lines and PR-authored test lines from about 1,180 to 1,670.
1. The Codex snapshot cursor goes past what #5053 decided. Decision B asks to reuse the Host paged catalog and keep the underlying reads bounded. What three rounds of "rows shift between pages" produced instead is a second paging interface (listSessionPage, page item type, ExternalSessionCursorExpiredError), an adapter-owned snapshot table with random tokens, a 5-minute sliding TTL and a 32-entry LRU, a new protocol error cursor_expired, a TUI clear-and-reload state with three locale strings, and for state_*.sqlite a BEGIN read transaction held open across pages on Codex's live database. About 290 production lines across five packages, defended by four tests. Measured on the WAL path: while a page is held, wal_checkpoint(TRUNCATE) returns busy and 2,000 writer commits left 2,000 frames unreclaimable; a user who opens the picker, reads page one and presses Escape pins Codex's WAL for the full five minutes, and nothing disposes the reader when the picker closes. On Windows the open handle also stops Codex rotating its state file. The TTL and LRU, the only bound on that hold, have no regression: with both removed the Codex suite is still 20/20.
The obligation the picker actually has is: no duplicate, stable order, bounded first page. Keyset paging meets all three with zero server state: order by (sort_ts DESC, id DESC), cursor is the last seen pair, WHERE sort < ? OR (sort = ? AND id < ?), carried in the existing opaque cursor string. Filesystem fallback does the same over (mtime, path). What it gives up is "a row updated mid-traversal stays at its old position", which nothing in #5053 asks for, and which Claude and OpenCode don't offer either since they're still on numeric offsets. The strongest test in the PR (external-session-coordinator.test.ts:147, real handler, real SQLite, concurrent UPDATE) pins that stronger guarantee rather than the obligation. I'd like the paging shape decided before anything else in this round, because cursor_expired, the TUI reload path, the WAL hold, the journal-mode source switch (non-WAL installs silently get the filesystem corpus with different titles and timestamps) and about 240 test lines all go with it.
2. Ablate everything added since 85c7070, not only what was named. You removed the five tests I listed and nine Desktop mechanism tests, but the twelve fix commits each brought their own, +664 lines, and several defend an intermediate revision of this PR rather than an obligation:
- Heavy and redundant:
opencode-session-adapter.test.ts:205pushes 2,048×2 rows into real SQLite (1.2 s, slowest test in the workspace) to prove what:326proves withmaxRows: 1in 2.5 ms; disabling the row check reds both.codex-session-adapter.test.ts:584seeds 2,000 rollouts plus a state DB and assertssnapshotMs < boundedMs * 8 + 50; a wall-clock ratio on a shared CI box isn't a contract, and it doesn't observe "no materialisation" at all. If the obligation survives, count reader calls on 20 rows. - Green on
mainalready:claude-code-session-adapter.test.ts:682(guards a record count that no longer exists anywhere, with a 2,002-record fixture),opencode-session-adapter.test.ts:183(defends a title bound against an earlier draft),:405(readOnly: truepredates this PR). - Subsumed:
claude-code…:612and:630by:654, which goes red under the same head and tail ablations;external-session-importer.test.ts:141by:166;model-history-timeline.test.ts:28by thesession-managerregression (neutering the option reds both); thefor (const externalOrigin of …)loop insession-manager.test.ts:8496runs 115 lines of setup twice for a rule that doesn't branch on it. - Unreachable by its own comment:
external-session-coordinator.test.ts:409covers the over-budget page branch (:444-460) that:443proves can't be reached; delete the branch, itsexport, the per-item cursor type and the test together (about 35 lines, ablation run, one failure and it's that test).
That's roughly 330 test lines now and 570 if the snapshot goes, leaving a keep list where each test proves one obligation through its owner and is red on old behaviour. On the production side, same pass: OPENCODE_TRANSCRIPT_MAX_CONVERTED_BYTES has no reachable trigger (preflight already caps source at 64 MiB; measured amplification at the worst row shape is about 1.6×, so the ceiling is ~100 MiB against a 256 MiB cap) and is only fired by the test-only option; since Claude and Codex carry the same construct on main, either take all three out in a follow-up or leave all three, but don't add a test for one. Also the duplicate .slice(0, MAX_ITEMS) at coordinator.ts:230 (already enforced at :212), claudeAssistantText passthrough, repairedAssistantIndex scanning the full ledger on every caller when only one sets the option (move both findIndex inside the if), the two TUI decoders of the same {operation, code} envelope, and doc comments that narrate this PR's revision history (claude-code…:80-85, opencode…:74-78).
3. Three defects, each with a small fix at the owner.
- P2, Codex ordering.
codexThreadQuery(codex-session-adapter.ts:1090) multiplies every non-_mscolumn by 1000 unconditionally;normalizeEpochMs(:1271), which produces theupdatedAtshown for the same row, multiplies only below 1e12. Whenupdated_atholds milliseconds, an hour-old Session sorts first and the newest falls off page one. OneCASE WHEN col >= 1000000000000 THEN col ELSE col * 1000 ENDfixes it (verified, 20/20), or delete the JS branch if seconds is the only supported unit; one rule either way. The current regression passes with either threshold, so it can't tell. - P2, Claude scoped catalog. When neither summary window yields a record (one record over 512 KiB, e.g. a pasted file as the opening prompt),
readTranscriptSummarydegrades tocwd: '', which then fails the workspace clause inexternalSessionMatchesQuery, and the catalog is always workspace-scoped. The comment says the fallback "must not hide a real source Session"; it does, completely. The adapter already knows theprojects/<encoded-cwd>directory; decode that for the degenerate row. - P2, Desktop batch import. hqhq1025's finding stands:
importSelected()(import-tasks-settings-page.tsx:676) builds targets frommarkedwithout consultinguncertainImports, and the checkboxes and button aren't gated on it. The underlying issue is that this lock is client-local and the TUI has none at all, so the two clients diverge again on the same Host outcome. The Host already keys in-flight imports by(adapterId, sourceSessionId); retaining a settled-unknown entry there is the owner-level fix, but it's a protocol change and I wouldn't block on it. For this PR: filter the batch targets the same way as the single path, or drop the Desktop lock to match the TUI, and say which.
P3s, no action needed unless convenient: an imported transcript with no user event at all replays as empty context while the UI shows the full conversation (firstUserIndex < 0 → []), worth a test; a malformed numeric cursor is handled three different ways by the three adapters (Claude loops on page one with NaN), validate once in the coordinator; the final page released on hasMore: false can still hand back a cursor when the wire budget truncates it.
Facts: 5 behind main, merges clean, lockfile unchanged, no hosted checks on this head. Storage, core, runtime-host, runtime and CLI focused suites pass here.
AI assistance: I used Claude Code to run the provider-path probes, the WAL and memory measurements, and the ablations; conclusions are mine.
中文版
感谢这么快推进。评审基于 head ae1f5a1,已 rebase、可合并。上轮的阻塞项在正确的 owner 处修好了:RuntimeLedgerRepair 只剩一条 turn 规则,provider 准入放在 model-history.ts,用 refs.storedMessageId 限定,而这个 ref 只有 repair 的 backfill 会写。我重跑了真实路径(SessionManager.sendMessage → buildPriorRuntimeContext → 带录制 model 的 AiSdkBackend):导入的 assistant 开头 transcript、原生 pre-ledger 的 assistant-only turn、thinking-only 开头、continuation 通道,线上都是 user 开头。我也试了粗糙规则(「丢掉第一个 user 之前的一切」):ai-sdk-backend 真实失败 22 个,因为 prior context 经常是预算/折叠后的切片,本来就合法地以 assistant 事件开头。所以 provenance 限定是必要的,不是多余。session-manager 那条回归在关掉选项后变红。上轮第 1、3、4 点的每一项都按要求做了。好。
这个 head 有两件事突出,和上次是同一个形状:每轮评审都在评审者指到的位置加机制和测试,消融只覆盖被点名的。生产代码相对 main 净减 374 行,方向对,但 Codex adapter 从 +100 涨到 +423 行,PR 自己写的测试从约 1,180 行涨到 1,670。
1. Codex snapshot cursor 超出了 #5053 定的范围。 决策 B 要的是复用 Host 分页目录、底层读取有界。三轮「翻页时行会移动」修出来的却是:第二套分页接口(listSessionPage、page item 类型、ExternalSessionCursorExpiredError)、adapter 自持的 snapshot 表加随机 token、5 分钟滑动 TTL 和 32 项 LRU、新协议错误码 cursor_expired、TUI 清空重载状态加三种语言文案,以及 state_*.sqlite 路径上对 Codex 活库跨页持有的 BEGIN 读事务。约 290 行生产代码跨五个包,由四条测试守着。WAL 路径实测:持页期间 wal_checkpoint(TRUNCATE) 返回 busy,2,000 次写提交留下 2,000 帧无法回收;用户打开选择器看一页按 Esc,Codex 的 WAL 就被钉满五分钟,选择器关闭时没有任何东西释放 reader。Windows 上打开的句柄还会阻止 Codex 轮换 state 文件。TTL 和 LRU 是这个持有的唯一上界,却没有回归:两个都去掉,Codex 套件仍是 20/20。
选择器真正的义务是:不重复、顺序稳定、首页有界。keyset 分页零服务端状态就满足这三条:按 (sort_ts DESC, id DESC) 排序,cursor 是最后看到的一对值,WHERE sort < ? OR (sort = ? AND id < ?),装进现有的不透明 cursor 字符串。文件系统 fallback 对 (mtime, path) 做同样的事。它放弃的是「翻页中被更新的行留在旧位置」,#5053 没要求这点,Claude 和 OpenCode 也没提供,它们还是数字 offset。PR 里最强的那条测试(external-session-coordinator.test.ts:147,真实 handler、真实 SQLite、并发 UPDATE)钉住的是这个更强的保证而不是义务。我希望这轮先把分页形态定下来,因为 cursor_expired、TUI 重载路径、WAL 持有、journal-mode 语料切换(非 WAL 安装会静默拿到文件系统语料,标题和时间戳都不同)和约 240 行测试都随它去留。
2. 对 85c7070 之后新加的所有东西做消融,不只是被点名的。 你删了我列的五条测试和 Desktop 九条机制测试,但十二个修复 commit 各自带了测试,+664 行,其中不少守的是这个 PR 的某个中间版本而不是义务:
- 重且冗余:
opencode-session-adapter.test.ts:205往真实 SQLite 塞 2,048×2 行(1.2 秒,workspace 里最慢的测试)去证明:326用maxRows: 1在 2.5 毫秒里证明的事;关掉行数检查两条都红。codex-session-adapter.test.ts:584生成 2,000 个 rollout 加一个 state DB,断言snapshotMs < boundedMs * 8 + 50;共享 CI 机器上的 wall-clock 比值不是契约,它也根本观察不到「没有物化」。如果这个义务还在,用 20 行数 reader 调用次数。 main上已经绿:claude-code-session-adapter.test.ts:682(守一个已经不存在的记录计数,还带 2,002 条记录的 fixture)、opencode-session-adapter.test.ts:183(防的是早期草稿的标题上限)、:405(readOnly: true早于本 PR)。- 被包含:
claude-code…:612和:630被:654包含,后者在同样的 head 和 tail 消融下变红;external-session-importer.test.ts:141被:166包含;model-history-timeline.test.ts:28被session-manager回归包含(废掉选项两条都红);session-manager.test.ts:8496的for (const externalOrigin of …)循环为一条不按它分支的规则把 115 行 setup 跑两遍。 - 注释自己承认不可达:
external-session-coordinator.test.ts:409覆盖的 over-budget 分支(:444-460)被:443证明进不去;分支、它的export、per-item cursor 类型和这条测试一起删(约 35 行,消融已跑,只有这条测试失败)。
现在大约 330 行测试可删,snapshot 去掉则 570 行,留下的每条都经过 owner 证明一个义务、在旧行为上变红。生产代码同样过一遍:OPENCODE_TRANSCRIPT_MAX_CONVERTED_BYTES 没有可达触发(preflight 已把源限在 64 MiB;最差行形状实测放大约 1.6×,上限约 100 MiB,对着 256 MiB 的 cap),只能靠测试专用选项触发;Claude 和 Codex 在 main 上有同样的构造,要么后续一起删三个,要么三个都留,但别只给一个加测试。还有 coordinator.ts:230 重复的 .slice(0, MAX_ITEMS)(:212 已经限过)、claudeAssistantText 直通、repairedAssistantIndex 在每个调用方都全表扫描而只有一个调用方设了选项(两个 findIndex 挪进 if)、TUI 里对同一个 {operation, code} 信封的两个解码器、叙述本 PR 修订史的注释(claude-code…:80-85、opencode…:74-78)。
3. 三个缺陷,各有一个 owner 处的小修法。
- P2,Codex 排序。
codexThreadQuery(codex-session-adapter.ts:1090)对所有非_ms列无条件乘 1000;normalizeEpochMs(:1271)给同一行产出显示用的updatedAt,却只在小于 1e12 时乘。updated_at存毫秒时,一小时前的会话排第一,最新的掉出首页。一个CASE WHEN col >= 1000000000000 THEN col ELSE col * 1000 END修好(已验证,20/20),或者如果只支持秒就删掉 JS 那个分支;两者取一条规则。现有回归对两种阈值都通过,分不出来。 - P2,Claude 工作区目录。两个摘要窗口都读不到记录时(单条记录超过 512 KiB,比如粘一个文件当开场提示),
readTranscriptSummary降级为cwd: '',然后过不了externalSessionMatchesQuery的工作区条件,而目录总是工作区作用域的。注释说这个 fallback「不能隐藏真实的源会话」,它把会话完全藏掉了。adapter 已经知道projects/<encoded-cwd>目录,给降级行解出这个 cwd。 - P2,Desktop 批量导入。hqhq1025 的发现成立:
importSelected()(import-tasks-settings-page.tsx:676)从marked构造目标,不看uncertainImports,复选框和按钮也没按它禁用。更根本的是这个锁只在客户端,TUI 根本没有,两端对同一个 Host 结果又不一致了。Host 已经按(adapterId, sourceSessionId)键控进行中的导入;在那里保留 settled-unknown 条目是 owner 级的修法,但那是协议改动,我不会拿它阻塞。这个 PR 里:批量路径像单条路径一样过滤,或者删掉 Desktop 的锁向 TUI 看齐,说明选了哪个。
P3,不强求:导入的纯 assistant transcript 发送时上下文为空而 UI 显示全部对话(firstUserIndex < 0 → []),值得一条测试;畸形数字 cursor 三个 adapter 三种处理(Claude 带 NaN 在第一页打转),在 coordinator 校验一次;hasMore: false 时已释放的末页在 wire 预算截断时仍可能交回 cursor。
事实:落后 main 5 个 commit,合并干净,lockfile 未变,此 head 没有托管检查。storage、core、runtime-host、runtime 和 CLI 的聚焦套件在我这里都通过。
AI 辅助:我用 Claude Code 跑了 provider 路径探针、WAL 和内存测量以及消融;结论由我负责。
ae1f5a1 to
ddd24e3
Compare
|
Addressed the latest review in
The design report now describes only the final implementation: #5308 (comment) Verification after rebase: all five affected workspaces build; focused storage 87/87, Runtime 197/197, Host/protocol 32/32, CLI 214/214, Desktop 35/35; renderer typecheck, Biome, and |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head ddd24e32fe89561314b949c9f26d3a7741eff6c9.
The previous assistant-first history failure and mutable offset pagination defect are fixed. This revision also consistently blocks duplicate uncertain/in-flight imports in Desktop and the TUI. The current head is still not ready because the new stateless Codex cursor has two reachable omission cases, and invalid opaque cursors are reported as storage failures.
Validation completed on Node 24.18.1: clean install, build:test, full workspace typecheck, 45 focused Storage/Runtime Host tests, CLI 214/214, Desktop import page 35/35, the assistant-first production regression, protocol/model-history 17/17, changed-file Biome, ASF headers, git diff --check, and a clean merge tree with current main dd15b63c60039a77ed980b27c3306af08ad1b9ee. GitHub reports MERGEABLE/BLOCKED with no hosted status checks.
Not covered: a real concurrent Codex process rotating state databases, or packaged Desktop interaction.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 6dd58dfb16a1e3abbfbd2567880e093d031a1b48.
I found no new P0-P3 issue in this revision. The three findings from the previous head are resolved on the production paths:
- Codex filesystem fallback now uses the same comparator for ordering and keyset continuation. A tied-mtime
codex_a/codex-aprobe visits both rows exactly once. - Codex database cursors now bind to the
state_*.sqlitegeneration that issued them. After page one fromstate_5.sqlite, adding a newerstate_6.sqliteno longer drops the remainingstate_5rows; a missing or unreadable issuing generation fails the cursor instead of silently switching corpora. - Source-owned cursor failures use
ExternalSessionCatalogCursorError, and the Host maps them toinvalid_requestrather thanpersistence_failed. Claude Code and OpenCode now expose the same opaque, query-bound paging contract through the shared offset adapter.
Validation on Node 24.18.1: clean install, build:test, full workspace typecheck, focused Storage/Core/Runtime Host tests (134/134 on the clean merge tree), full Storage (1362 pass / 11 skip), changed-file Biome, formatting, ASF headers, and git diff --check. The merge tree with current main 2c49a9986fc4cbc6f09c1afb2c9d205b67c86a7b is clean and its build plus focused tests pass. The full Runtime Host suite reached 1945 pass / 19 skip / 1 fail; the sole failure is the existing managed-Bash sandbox case on this runner, where namespace/sandbox execution is unavailable, and is outside this change.
GitHub currently reports MERGEABLE/BLOCKED with no hosted status checks. I did not test a packaged Desktop build or a real Codex process rotating its state database during an open picker. This is a feature change, so the merge decision remains with a human maintainer.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
e702ee1 to
82f90d6
Compare
There was a problem hiding this comment.
Thanks for the sustained iteration — the keyset paging, the scoped admission rule, and the ablations all landed where they should. Reviewed at head 82f90d6d. This round is organized differently on purpose: six rounds of "close finding N" converged each time because each list was finite, but the remaining defects are not a list — they are instances of a few unstated invariants. So instead of another findings list, this review states each invariant, names its single owner, and audits all of its consumers once. The goal is that fixing the owner closes the whole class, not the instance a reviewer happened to point at.
Verified on this head before listing anything: the durable/import path is sound end to end (single transaction, hidden v0 staging, awaited recovery, idempotent rematerialization); the keyset contract is exact (no dup/skip on budget cuts, generation-bound cursors); the provider wire is user-led on the send path (traced through buildPriorRuntimeContext); the deleted scanner/digest surface left no dangling references.
Invariant 1 — every ledger→provider projection is user-led
Invariant. Any projection of RuntimeEvents into provider messages must not emit repaired backfill (refs.storedMessageId) content before the first model-visible user event. Explicit continuations are admitted separately.
Owner. buildRuntimeEventModelReplayPlan. The rule currently exists as the opt-in flag startAtFirstUserBoundary, enabled only in ai-sdk-turn.ts. An invariant that each caller must remember to request is not an invariant — it is a convention, and this PR's own history shows conventions get forgotten.
Consumer audit (every caller that can see a repaired ledger):
| Caller | Boundary applied? | Consequence on an assistant-first repaired ledger |
|---|---|---|
ai-sdk-turn.ts:2797 (send) |
yes | user-led — verified |
| continuation lane | separately admitted | provider_resume_head_unsupported |
history-compact-summarizer.ts:126 |
no | first fold sends [assistant, …] → provider 400 → compaction fail-open latches for the session's lifetime |
ai-sdk-compaction.ts:1054 (mid-turn) |
no | same |
runtime-kernel.ts:1227 (explicit compact) |
no | same |
memory-extraction.ts:1710 |
no | assistant-first source context → counted failure |
session-recap.ts:117 |
no | silent ok:false |
This defect class technically predates the PR ([assistant, user] repairable turns existed), but zero-user turns are the shape this PR introduces, so the exposure goes from corner case to target scenario. Smallest fix: make the boundary the projection's default and invert the option for the admitted continuation lane — then no present or future consumer can forget it. Alternative, weaker: pass the option at each consumer. Either way, the review criterion is the table above closing to all-yes.
P2. Also small: when the slice drops the prefix, emit a repaired_prefix_dropped diagnostic — today an assistant-only transcript silently projects to [] while the UI shows a conversation.
Invariant 2 — a catalog page is a bounded slice of one stable total order, resumable by position
Obligations. No duplicates, no silent omissions, bounded first page; the cursor names a position in the order (keyset), not a copy of the corpus; validation happens once at decode; and the error taxonomy distinguishes cursor invalid (client input, invalid_request) from source unreadable (transient, persistence_failed).
Consumer audit:
- Dead path. Codex still carries the replaced offset engine —
listSessions→listCatalog→scanRolloutCatalog→walkRolloutFilesplusreadCodexThreadRows'spageparameter — extended this round and pinned by new tests, all unreachable in production (codex-session-adapter.ts:144,173-205,244-276,899-920). The interface the coordinator consumes islistSessionPagealone. Per repo rule the replaced mechanism leaves in the same PR; repoint the still-valuable assertions (mixed-unit ordering, filters) atlistSessionPageor delete them. P2. - Misclassified transient failure.
readStateCatalogKeysetPageswallows every read error intoundefined, whichlistCatalogKeysetPageturns intoExternalSessionCatalogCursorErroron continuation pages (codex-session-adapter.ts:344,289-291→ coordinator mapsinvalid_request). ASQLITE_BUSY/checkpoint race while Codex writesstate_N.sqlite— normal operation — hard-fails Load More on a cursor that is still valid. Returnundefinedonly for "no usable threads table" and let read errors surface aspersistence_failed. P2. - Unbounded per-page I/O.
nextRolloutCatalogBatchhead-reads (≤512 KiB + parse + realpath) every post-keyset candidate on every page because traversal order is not sort order (:1068-1094). The file already contains the correct shape —scanRolloutCatalogsorts by stat-known keys first, then reads heads only untillimitmatches. P2 on a no-state-DB corpus, otherwise P3. - Guarded crash path.
boundedCatalogPageevaluatescandidates[index-1]!atcoordinator:417; index 0 would be aTypeError→persistence_failed. The "one row always fits" invariant is test-pinned, so either keep it and say so at the!, or handle index 0 explicitly. P3.
Invariant 3 — one outcome, one meaning, on every client
Invariant. "Dispatched but unanswered" has exactly two wire shapes: the Host error code commit_outcome_unknown, and RuntimeHostRequestInterruptedError with dispatch === 'dispatched'. Both mean: unconfirmed, never retried blind, never attributed to another client's import. Everything else maps by its own code.
Consumer audit: the TUI decodes both shapes (pi-tui-runner.ts:329). Desktop main maps only the operation error — an interrupted import throws through runtime-host-external-sessions-ipc-main.ts:97-121, renders as a generic failure, leaves the row eligible, and a retry can duplicate a task that did land. That is the exact fail-open case this PR exists to remove, and it makes the two clients disagree on one Host outcome again. Pre-existing, but the seam is this PR's contract and the fix is ~6 lines plus emitSessionsChanged('created'). P2.
Residual to decide, not silently accept: a post-dispatch error that is neither shape (e.g. a rejected response frame) is still treated as retryable on both clients. onboardingSave maps even non-envelope errors to outcome-unknown for this reason. Pick one and document it. P3.
Invariant 4 — source bytes cross exactly one canonicalization boundary before persistence
Invariant. Before anything is durable: titles pass sanitizeExternalSessionTitle (incl. redactSecrets), cwd is bounded and control-char-free, ts is a non-negative safe integer, and failures carry typed errors. The commitAttempted flag must sit at the durable boundary — validation failures are provably pre-commit and must never report commit_outcome_unknown or drain the host.
Audit of what crosses today:
commitAttempted = trueprecedes validation insidecreateImportedSession(coordinator:283-287): a rolled-back transaction — non-canonical message, name sanitizing to empty,message_ts >= 0CHECK (all three adapters passts < 0through), id collision — reports unknown +requestDrainfor a deterministic nothing-was-written failure. P2.- OpenCode
readSessionpersistsrow.titleraw (opencode-session-adapter.ts:172) while the catalog sanitizes the same field (:458) — a secrets-shaped title is stored unredacted, and a control-char-only title throws inside name normalization after the flag above. P2. metadata.cwdis unbounded: Claude takesrecord.cwdverbatim up to the 64 MiB record cap (claude-code-session-adapter.ts:340), Codex strips control chars but caps nothing, OpenCode caps at 4 KiB in preflight only. It lands in the header and the ledger'sconfiguration.cwd. One shared bound in the importer covers all three. P2.- Codex limit failures throw plain
Error(:731-735,752,839) sosource_limit_exceedednever fires for Codex — the typed result and its UI copy exist unused. P2. isSourceSessionNotFoundgreps English text (coordinator:457): Claude throws "transcript not found" which does not match, so a delete-between-list-and-import race reportssource_unreadable. One typedExternalSessionNotFoundErrorin the adapters replaces the regex. P2 (pre-existing, in-seam).
Invariant 5 — state the data assumption before the algorithm depends on it
readTurnsInPages assumes turnIds are contiguous: it carries only the last-inserted group, not the group owning the page's last row (runtime-ledger-repair.ts:182-199). Codex terminal rows use payload.turn_id, which can reference an older turn — interleaved rows across a page boundary either convert one turn twice under colliding ids or persist a wrong terminal verdict. Reachability on real Codex output is unproven: please pin it with an interleaved fixture; if reachable, carry all still-open groups rather than one. P2 if proven, otherwise document the assumption.
Regression vs the deleted flow
The deleted TUI scanner let users search (SessionSearchOverlay covered title/id/cwd). The new picker is a bare SelectList (pi-tui-runner.ts:3120-3147) and the surface has no text (pi-tui-contracts.ts:207) — while the Host query and Desktop already support it. Client-side filtering cannot see unloaded pages, so parity requires the wire field. P2.
Smaller items (P3, fix if convenient): TUI drops ineligible rows entirely where Desktop disables them with a banner — pick one model; the busy guard reports "import failed" when nothing was attempted; external-session-coordinator.ts:397 keeps a dead number union member; a Desktop comment at import-tasks-settings-page.tsx:158 still documents the removed recovery mechanism; session.ts:806 still says "foreign".
How to close this
For each invariant above, the acceptance is the audit table going green — not the named line changing. If a fix lands anywhere other than the stated owner, please say why. Most of items 3–5 are pre-existing; this PR extending the surface is what exposed them, and the seam-local ones are cheap here — but it is fine to split the rest into follow-ups as long as the review says which.
Head 82f90d6d, merges clean into main 27add3049f, no hosted checks on this head. I did not exercise a packaged Desktop build or a live provider call.
AI assistance: this review used delegated agents to audit each contract boundary independently; I verified the load-bearing claims against the head source myself.
中文版
评审基于 head 82f90d6d。前几轮"定点修复"每轮都收敛,是因为清单有限;剩下的缺陷不是清单问题,而是几条未被陈述的不变量在不同消费者身上反复长出来。本轮按不变量组织:每条给出不变量、唯一 owner、一次性消费者审计,验收标准是审计表全绿而不是某行被改。
- Provider 历史必须 user 开头:规则做成了 opt-in 选项,只有 send 路径开了;compaction summarizer / mid-turn compact / explicit compact / memory / recap 五个投影方没开,assistant-first 账本会让首个 fold 拿到 assistant 开头的 messages → provider 400 → compaction 永久失效。最小修法:把边界做成投影层默认行为,continuation 显式豁免。
- 目录页是稳定全序上的有界切片:Codex 被取代的 offset 引擎未删还带着新测试;续页 catch-all 把瞬时读失败误报 invalid_request;FS keyset 每页对所有后续候选读 512KiB head。修 owner:删死路径、错误分类归位、先按 stat 排序再读。
- 同一结果两端同义:dispatched-丢失有且仅有两种 wire 形状(code + interrupted/dispatched);Desktop main 只解一种 → 失败可重试 → 可能重复导入。~6 行修复。
- 源字节过一个规范化边界才持久化:commitAttempted 在校验前置位(回滚失败误报 unknown+drain);OpenCode title 裸存不脱敏;cwd 无界;Codex 超限不抛 typed error;not-found 靠 message 正则。多数预存,但在本 PR 扩展的缝上。
- 先陈述数据假设:
readTurnsInPages假设 turnId 连续,Codexpayload.turn_id可乱序;先给 fixture 证可达,可达则携带所有未闭合组。 - 回归:TUI 丢了旧流程的文本搜索,wire 已支持
text,接上即可。
其余 P3:切片丢前缀无 diagnostic;TUI 整行隐藏 vs Desktop 禁用(选一个模型);busy 误报文案;死 union 成员;两处陈旧注释。
|
@Astro-Han Thanks for the invariant and authority review at
The complete, versioned PR design is available in English and 中文. Verification after the interaction changes: |
Generated-by: Codex
Generated-by: Codex
a6715d4 to
47a7db7
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at head f438e90af — the branch was rebased onto current main and gained three commits on top of what I last saw (47a7db75e protocol re-pin, 5ebd5f04c renderer ledger sync, f438e90af settle turns from their final state). I verified each invariant from the previous round against the head source rather than re-running a findings list:
| Invariant | Status at f438e90af |
|---|---|
| User-led provider projection | Closed — the boundary is now the default inside buildRuntimeEventModelReplayPlan (model-history.ts:613); the only opt-outs are the admitted-continuation lanes (ai-sdk-turn.ts:2806, continuation-replay.ts:174, runtime-kernel.ts:3180), applied symmetrically on both digest sides, so PROVIDER_REPLAY_PROJECTION_VERSION correctly stays 2. The two parallel projections apply it explicitly (session-recap.ts:44, memory-extraction.ts:244, which exempts itself only when a text checkpoint supplies the user-led head). repaired_prefix_dropped is non-blocking in both diagnostic classifiers. |
| Bounded catalog slice | Closed — the offset engine is gone with no residual tests pinning it, cursor vs transient-read errors classify correctly, the FS fallback orders by stat-known keys before head-reading, and the index-0 path is guarded. |
| One outcome, one meaning | Closed — both clients decode both uncertain shapes and fail closed on every residual post-dispatch error; not_dispatched is the only retryable interruption; the re-read survives connection teardown. |
| Canonicalization before persistence | Closed — onCommitStarted fires at the durable write (session-store.ts:216); title/cwd/ts canonicalize in one importer boundary covering all three adapters; typed ExternalSessionNotFoundError replaced the regex; Codex limit failures throw source_limit_exceeded. |
| Stated data assumption | Closed — readTurnsInPages now settles each turn at its final sequence (the new two-pass commit also closes the turnId-reuse residual from last round), with a real cross-page interleaving fixture. |
| TUI text search | Closed — wire text reaches the picker and stale responses are revision-dropped. |
The audit table went green, which is the acceptance bar this review set. What remains is one merge blocker on the PR's own test surface plus a short list of new, mostly local defects introduced by the fix commits — two P2s as inline comments.
Merge blocker — the test check fails deterministically
Storybook smoke: product-settings-pages--import-tasks-outcome-unknown-recovered asserts '已确认导入'/'Import confirmed' — copy that no longer exists anywhere in src. The story predates the §11 redesign: recoverUnknownImport used to surface a positive "confirmed" state, while the current model renders the importOutcomeUnknownTitle banner and lets the row's imported N times annotation carry the landed signal. If the removal was deliberate, update the story to assert the new semantics (banner plus the row's post-refresh annotation); if it wasn't, the positive confirmation needs restoring. Either way the check keeps failing until one of those happens.
New findings
P2 (pre-existing, in-seam) — ExternalSessionCoordinator.recover() (external-session-coordinator.ts:136-143) awaits #prepareStagedSession for each v0 header with no failure isolation: a staged session whose prepare deterministically fails and whose discardImportedSession also fails propagates out of recoverRuntimeHostDomainModules, so one bad staged session keeps the whole host from ever reaching ready. Narrow double-fault trigger, maximum blast radius. Collect the failure or quarantine the header rather than aborting all domain startup.
P3 — Emitted filesystem cursors can exceed the 512-byte wire bound on deeply nested rollout paths (encodeCatalogKeyset embeds the full base64 catalogKey; decode caps at 320B/512B): a user-managed backup tree under sessions/ fails the very page that emitted it. Bound catalogKey length at enumeration or hash it.
P3 — TUI catalog search fires a Host-side scan per keystroke (pi-tui-runner.ts:3195); responses are revision-guarded so correctness holds, but Desktop debounces 250ms for the same query. Worth the same coalescing, plus a test pinning text forwarding.
P3 — The old TUI search matched the source session id as well as title/cwd; the wire matcher covers title+cwd only (external-session.ts:146-148), so pasting a Codex UUID no longer finds the row. One-line fix in externalSessionMatchesQuery that also benefits Desktop.
P3 — projectSessionCatalogMessages(canonicalMessages) is evaluated as an argument after onCommitStarted?.() fires (session-store.ts:216-220): a deterministic throw there reports commit_outcome_unknown + drain with nothing written. Compute the projection before firing the callback.
P3 — The memory test double's import lookup lacks the transcriptLedgerVersion <> 0 filter the SQLite store applies, so tests on the double cannot reproduce staged-row invisibility — the property the recovery contract depends on.
Head f438e90af, mergeable but BLOCKED on the failing test check. I did not run the suite locally or exercise a packaged build.
AI assistance: verification used delegated agents auditing each contract boundary independently; the load-bearing claims (boundary default + continuation digest symmetry, the two P2s, the stale story) were re-verified against head source before publishing.
中文版
在 head f438e90af 上重新评审(rebase 后新增三个提交也已覆盖)。上一轮六条不变量逐条对照 head 源码核验,全部落地:投影边界成了 planner 默认行为且 continuation 两侧对称豁免(版本号正确地保持 2)、offset 引擎连测试一起删干净、两端客户端对所有 dispatch 后错误形状都 fail-closed、commit 标记落在真正写库那一步、规范化收口到单一边界、turn 按最终 sequence 结算并带真实交错 fixture、TUI 搜索走了服务端 text。
剩下的问题:
- 合并阻塞:CI
test确定性失败——story 断言的"已确认导入"文案在新模型里已不存在(旧 recoverUnknownImport 有正向确认态,§11 重设计改成了"需要确认导入结果"横幅 + 行内"已导入 N 次"标注)。要么改 story 断言新语义,要么恢复正向确认;二选一之前 CI 一直红。 - P2 行内 ×2(catalog 合并新引入):① 首个 state DB 损坏会让所有无游标目录查询失败——循环只对
undefined容错,throw 直接传播,而findCatalogEntry对同一文件仍容错;② 排序键 SQL 里算一份、JS 里算一份,TEXT 排序列上两者分叉(ISO 文本在 numeric affinity 列下 SQL 键变成 ~2026000 而游标 ~1.7e12 → 每页重复、hasMore永不清;反向则静默丢行)。最小修法:SELECT … AS sort_key一个权威。 - P2 正文(预存、缝内):
recover()对 v0 header 逐个 await,prepare 失败且 discard 也失败时整个 Host 启动被拒、永远到不了 ready——触发条件窄但爆炸半径最大。 - P3 若干:深嵌套路径的 FS 游标可超 512B wire 上限;TUI 每键一次扫一次 Host(无 debounce,正确性有 revision 守卫兜底);源 session id 不再可搜;
projectSessionCatalogMessages在 commit 标记之后才求值;内存测试替身缺transcriptLedgerVersion <> 0过滤。
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at f438e90af. The six invariants from my earlier reviews are closed end to end (see the verification table in the previous review), and the remaining P2s are localized and follow-up-able rather than blocking — the fail-closed direction is already correct everywhere, so none of them can silently corrupt or duplicate data.
Two things still gate the merge mechanically, not on review grounds:
- The
testcheck is red onImportTasksOutcomeUnknownRecovered— a stale story assertion (see previous review). Trivial fix: assert the §11 banner +imported N timesannotation. - Suggest filing the P2s as follow-ups before merging so they don't get lost: corrupt-state-DB fallback defeat, the SQL/JS sort-key divergence,
recover()failure isolation.
中文版
通过。上一轮六条不变量已全部闭环,剩余 P2 都是局部问题且方向已是 fail-closed,不会造成静默损坏,适合后续跟进。合并前只剩两个机械性门槛:CI 里那个断言已删除文案的 story(一行断言修复),以及建议把两条 P2 先立 follow-up issue 防丢。
|
@Astro-Han Thanks for the invariant-based re-review. Follow-up is now at head
Local verification on the exact head:
I replied to and resolved the two outdated inline P2 threads. The remaining recovery-isolation P2 is tracked in #5401, and the independently reviewable P3 hardening slices are tracked in #5402; both are assigned to me. The new hosted workflows are still |
Rebuild the full-load transcript on main's per-event ordinals (apache#5365). Running Turns are now durable pages, so the Desktop overlay (replica overlay, overlay bootstrap page, loadTranscriptOverlay, fragment source) is removed rather than carried forward, and the protocol epoch moves to 160 after apache#5308 took 159; its compatible-change declaration is re-pinned. Review fixes re-checked under main's model: - The Turn boundary marker is computed per scan: a run starts between Turns only when every Turn the walk has entered lies behind it. Runs are single-invocation stretches, so a change of owner no longer says a page is between Turns when Turns nest. - A reset still rereads down to the oldest sequence the consumer was given, because a reset replaces what the reader holds. - The guest transcript reader now passes its projection through to readPage; before, guests saw unprojected rows. Covered by a reader test. - The test for rows published below the watermark is dropped: every committed event takes MAX+1, so that premise no longer holds. Generated-by: Claude Code
Summary
Unify how the TUI and Desktop App continue an external Session through the Runtime Host catalog/import path. Every explicit import creates an independent native Maka Session snapshot, and importing sends no model request. The next user message continues the imported Session.
The Host and Storage remain the authority for the published import count, recent imported Session IDs, and in-flight imports. In the TUI, selecting a source with a previous import offers Open latest imported task or Import again; Desktop retains its separate row actions. An unknown import outcome is shown as a warning, while the user can inspect the task list or explicitly import again. Neither client stores an unknown lock or attributes a catalog record to an unanswered request.
The catalog also stops failing as a whole when a Codex state database cannot be read. An unreadable newest generation is now answered by the rollout scan rather than by an older generation, which would silently drop every Session created since the last bump. The
d:cursor path stays strict: a cursor names its generation and must fail rather than switch corpora.The Codex keyset ordering key is now computed once and read back to build the cursor, so a cursor cannot name a position the query did not order by.
Current behavior and module ownership: English design · 中文设计.
Refs #5053
Verification
Verified at the previous head,
f438e90af:npm run build:testandnpm run typecheckpass across all workspaces.main9982e86b1, 11 focused TUI, Desktop, Host, and Storage suites pass (469/469; no skipped tests).git diff --checkpass; commit hooks pass. Changed-file Biome passed before the rebase and was not rerun for the documentation/epoch-only follow-up.Verified on the three commits added on top (
0a8bba6e1,1556ab18c,c083dd157):npm run typecheckpasses; changed-file Biome passes.codex-session-adapter27/27, no skipped tests. Each of the three new cases fails without the change it pins.external-session-coordinator24/24 andexternal-session-protocol11/11.product-settings-pages--import-tasks-outcome-unknown-recoveredpasses the Storybook render smoke, which is the check that was failing, as do all 59product-settings-pages--*renders. Ablated back to the previous assertion, the story fails.test:dist: 1377 pass / 1 fail / 8 skipped on this runner. The failure ismanaged-dependency-environment-crash, which still fails with these commits stashed, so it predates them.context-offload-storefailed in one parallel run and passed in isolation and in the next full run. The reviewer's clean-tree run of the same suite reported no failures.~/.codexcatalog (685threadsrows) on this head and onf438e90af: identical result sets, 16 items each, same IDs and order, no duplicates, terminating.Not run: native packaged Desktop, Windows/macOS UI, or a real concurrently-writing external client. No live model call was needed for import. The
sort_keycast was reasoned fromsqlite3type-affinity probes rather than from a real Codex write of an unparseable timestamp, which I could not produce.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex and pi implemented the unified import path, review fixes, verification, and current design report.
Checklist
Does this PR entail a change in behavior?